interop-test: add interop test open telemetry tracing flags#11674
interop-test: add interop test open telemetry tracing flags#11674YifeiZhuang wants to merge 3 commits intogrpc:masterfrom
Conversation
|
Why does this need to be committed to unblock a one-off benchmark? If we're wanting these in general for tracing testing, then we would need to update the cross-language specification. I do also wonder if we should use the otel-defined environment variables for specifying the propagator instead of making something of our own. Given the interplay between otel metrics and tracing, it seems we should maybe have a flag to enable otel and then use otel-defined variables to configure which parts of otel are being used. |
|
@apolcyn, @yashykt and I had discussion. For one-off benchmark test, it would be easier to run an existing benchmark framework that uses interop test client here. The future work of otel tracing interop test (go/grpc-otel-tracing-acceptance-criteria) is likely different, i.e. it would require the test server to return a response on payload, and it would require the client to add a new a new test. This one is more about just enable opentelemetry tracing. I think it does not hurt to add one in Java for quick experimentation of C2P communication with otel tracing.
It's probably a good idea. Let me try using the environmental variable configuration. |
But what about that needs this change to be committed? |
Personally I am ok without this being committed. Committing will likely be useful in the future if/when we turn it on in continuous tests, or if we plan to do a number of manual experiments with this. But I'm fine to wait for that, too. |
Okay. Sounds good. |
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
This seems close to the right shape. We'd still want it defined cross-language, though.
| soakResponseSize = Integer.parseInt(value); | ||
| } else if ("additional_metadata".equals(key)) { | ||
| additionalMetadata = value; | ||
| } else if ("use_open_telemetry_tracing".equals(key)) { |
There was a problem hiding this comment.
This is more than just tracing, so I thought it'd be named more like "use_open_telemetry"
|
|
||
| void set(@Nullable Metadata carrier, String key, byte[] value) { | ||
| logger.log(Level.FINE, "Setting binary trace header key={0} value={1}", | ||
| new Object[]{key, Arrays.toString(value)}); |
There was a problem hiding this comment.
This toString() is performed even when logging is disabled. Does Arrays.asList() work?
| .sdk(openTelemetrySdk); | ||
| InternalGrpcOpenTelemetry.enableTracing(grpcOpentelemetryBuilder, true); | ||
| GrpcOpenTelemetry grpcOpenTelemetry = grpcOpentelemetryBuilder.build(); | ||
| // Disabling census-tracing is necessary to avoid trace ID mismatches. |
There was a problem hiding this comment.
The auto-applying is still causing trouble... This is a symptom of larger issues. It's okay for a while, but our users may have to deal with the same.
| OpenTelemetrySdk openTelemetrySdk = AutoConfiguredOpenTelemetrySdk.builder() | ||
| .addPropagatorCustomizer((reader, config) -> { | ||
| if (config.getString("otel.propagators") == null) { | ||
| return GrpcTraceBinContextPropagator.defaultInstance(); |
There was a problem hiding this comment.
We should set up the SPI for GrpcTraceBinContextPropagator.
3 participants
This is a quick change to unblock benchmark test: go/c2p-w3c-benchmark
Usage:
revision1:
./run-test-client.sh --use_tls=false --use_open_telemetry_tracing=true --open_telemetry_propagator=w3c./run-test-client.sh --use_tls=false --use_open_telemetry_tracing=true --open_telemetry_propagator=grpc-trace-binrevision2:
use w3c:
JAVA_OPTS=-Dotel.propagators=tracecontext ./run-test-client.sh --use_tls=false --use_open_telemetry_tracing=trueuse grpc-trace-bin(do not set otel.propagator):
./run-test-client.sh --use_tls=false --use_open_telemetry_tracing=truecc. @yashykt